-
Notifications
You must be signed in to change notification settings - Fork 804
Reformat gradle files with IntelliJ / .editorconfig #4037
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Unlike #4009 , there is no cost/time overhead (very high for "greclipse" !) or build complexity (however small) to maintain. True that there will be some inconsistency but we have a .editorconfig which should reduce that, especially amongst IntelliJ users. This is an improvement! |
| def resolved = configurations.javadocs.resolvedConfiguration | ||
| resolved.resolvedArtifacts.each { artifact -> | ||
| resolved.resolvedArtifacts.each {artifact | ||
| -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't love this newline left of the arrow; rather inconsistent. I fiddled with it now and it's appears to be because there are lines below that exceed 100 chars. Tweaks to them result in keeping the newline as it's been, which is nicer. I'll push a fix.
by reflowing to the 100 char width budget. Also removed a couple cases where we didn't need to pass an arg to the lambda.
|
I plan to merge this Monday night, subject to further review. |
|
I agree, greclipse is a monster. I remember thinking this kind of costly linter/ formatter stuff could be done on nightly workflows automatically - if the code compiles. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coming from the official Kotlin code conventions there are a few rules I'd like to adjust, but I am not sure which editorconfig rule(s) are required for these.
- Parameters of functions that exceed the max line length should be chopped down. They are currently wrapped at the point where max line length is exceeded
- Add space after the opening and before the closing braces, and write closing braces in new line (
ij_groovy_spaces_within_braces = trueand probably another rule) - Chop down arrays, lists etc. rather than wrapping, see example. It seems that the intellij formatter does not have any working rule for groovy
- Chop down chained method calls when too long (this one also has multiple rules work together probably, but still not nice format)
- Closing parentheses in multi-line function calls should be written in new lines (see point 1 example)
For 1. we often have this:
// Current formatting
ant.concat(destfile: "${outputDir}/stylesheet.css",
append: "true",
fixlastline: "true",
encoding: "UTF-8") { /*...*/ }
// Could be written as
ant.concat(
destfile: "${outputDir}/stylesheet.css",
append: "true",
fixlastline: "true",
encoding: "UTF-8",
) { /*...*/ }For 2. it is a bit more readable when there is a space here:
.configureEach {Task task -> task.doSomething()}
// could be
.configureEach { Task task -> task.doSomething() }For 3. we could do this:
// current
vectorIncubatorJavaVersions =
[JavaVersion.VERSION_21, JavaVersion.VERSION_22, JavaVersion.VERSION_23, JavaVersion
.VERSION_24, JavaVersion.VERSION_25] as Set
// Improved form
vectorIncubatorJavaVersions = [
JavaVersion.VERSION_21,
JavaVersion.VERSION_22,
JavaVersion.VERSION_23,
JavaVersion.VERSION_24,
JavaVersion.VERSION_25,
] as SetFor 4:
// Current formatting when too long
sources +=
sourceSet
.java.srcDirs.findAll {dir -> dir.exists()}.collect {dir -> relativize(dir)}
sources +=
sourceSet
.resources
.srcDirs.findAll {dir -> dir.exists()}.collect {dir -> relativize(dir)}
// Could be instead
sources += sourceSet.java.srcDirs
.findAll { dir -> dir.exists() }
.collect { dir -> relativize(dir) }
sources += sourceSet.resources.srcDirs
.findAll { dir -> dir.exists() }
.collect { dir -> relativize(dir) }This formatting rules are of course just a preference that I believe provide better readability. I could point out each place individually, but ideally I'd like to have the formatter / linter take care of that instead. I have also tried to see if I can find anything else, but for groovy it seems there is none.
Perhaps that is one more reason to start migrating from Groovy to Kotlin DSL, where we can use spotless again with ktlint and official standards (or more fine-grained custom rules). 🤞
| opts << | ||
| ['-bottom', "<i>Copyright © 2000-${project.buildYear} Apache Software Foundation. All Rights Reserved.</i>"] | ||
|
|
||
| opts << | ||
| ['-tag', 'lucene.experimental:a:WARNING: This API is experimental and might change in incompatible ways in the next release.'] | ||
| opts << | ||
| ['-tag', 'lucene.internal:a:NOTE: This API is for internal purposes only and might change in incompatible ways in the next release.'] | ||
| opts << | ||
| ['-tag', "lucene.spi:t:SPI Name (case-insensitive: if the name is 'htmlStrip', 'htmlstrip' can be used when looking up the service)."] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally like it if the opening parentheses / brackets are on the same line and the content then goes to the next line, with the closing parentehses at the last new line. Like
opts << [
'-tag', 'lucene.experimental:a:WARNING: This API is experimental and might change in incompatible ways in the next release.'
]
opts <<
['-tag', 'lucene.experimental:a:WARNING: This API is experimental and might change in incompatible ways in the next release.']But it is a personal preference and takes more lines of code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm now making newlines manually and IntelliJ isn't correcting me :-)
| exclude group: 'com.google.code.findbugs', module: 'annotations' | ||
| // Use Spotbugs Annotations as replacement | ||
| exclude group: 'javax.annotation', module: 'javax.annotation-api' | ||
| // Replaced with jakarta.annotation-api | ||
| exclude group: 'org.slf4j', module: 'slf4j-log4j12' // don't include log4j 1.x | ||
| exclude group: 'org.apache.yetus', module: 'audience-annotations' // Don't need annotations | ||
| exclude group: 'org.codehaus.mojo', module: 'animal-sniffer-annotations' // Don't need annotations | ||
| exclude group: 'org.codehaus.mojo', module: 'animal-sniffer-annotations' | ||
| // Don't need annotations |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Auto-formatting puts trailing comments below, but I think it is better if they are above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks; this very much needed correcting
|
For the most part, I've found IntelliJ to retain the newlines chosen. So we can format that way if you wish (manually). It's not clear the editorconfig/intelliJ settings are smart to what you suggest automatically. Except...
But I disagree with this suggestion. When I produced this editorconfig, I intentionally aligned groovy settings with that of Java, as groovy is basically a Java derivative. Consistency across our formats is more important IMO. |
They aren't. I spend some time to see if I could adjust a few rules so that they do what I expect, but without success.
Definitely, please go with consistency. I just dont remember the code formatting we use in the project and I completely went with the formatting that I use in Kotlin. Maybe formatting and linting is another reason why we should consider migrating from Groovy to Kotlin DSL? As far as I remember it would come with spotless support and good standards in formatting, plus some additional benefits like code completion and compile-time errors. Not sure where my original message ended up or if it was never published, so I tried to rewrite what I initially had |
# Conflicts: # solr/solr-ref-guide/build.gradle # solr/solrj-zookeeper/build.gradle
Includes some hand-edits. Not using Spotless because "greclipse" is very costly, and had some inconsistent inexplicable results that looked unpleasant. Follow-up commit will add changelog and `.git-blame-ignore-revs` addition
I told IntelliJ to reformat all gradle files. It uses the rules in our
.editorconfig.Also did a little hand-editing.
Once this is merged, I will record the commit in
.git-blame-ignore-revswith a changelog entry with the title:Reformat gradle files with IntelliJ / .editorconfig